chore: fix test warnings across test suite#1844
chore: fix test warnings across test suite#1844WilliamBergamin wants to merge 3 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1844 +/- ##
=======================================
Coverage 83.99% 83.99%
=======================================
Files 117 117
Lines 13252 13252
=======================================
Hits 11131 11131
Misses 2121 2121 ☔ View full report in Codecov by Sentry. |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin I left a question about the updates to validation script that give me some pause to approval. Please do let me know if I'm overlooking something in updates 🙏
| # all: ./scripts/run_validation.sh | ||
| # single: ./scripts/run_validation.sh tests/slack_sdk_async/web/test_web_client_coverage.py | ||
|
|
||
| set -e |
There was a problem hiding this comment.
🔭 question: Keeping this would cause this script to exit with error code to break CI I thought? Would this be related to deprecation changes otherwise?
There was a problem hiding this comment.
Good question, in CI we don't use this bash script, it's only used by maintainers locally
The script exits with error if it can't install a dependency or run a command, this is problematic when trying to test things locally against python 3.7 since some dependencies like black will fail to install and run.
This allows the scripts to "fail" formatting but still run the unit tests with python 3.7, there might be a better way to do this 🤔
There was a problem hiding this comment.
@WilliamBergamin Might we move this line with -e to before we run the tests but after linting happens?
I understand now this isn't used in CI but might still find it confusing in development - often exit codes help me build confidence 🧰
There was a problem hiding this comment.
I've updated the script so that formatting, linting and typechecking only run when using the latest supported python version and placed -e back into the script
There was a problem hiding this comment.
@WilliamBergamin Thanks for these changes! I like the preference to latest version for formatting so much!
| pip uninstall -y $package | ||
| pip uninstall -y $package & | ||
| done | ||
| wait |
There was a problem hiding this comment.
⏳ praise: TIL that not all must be rushed.
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@WilliamBergamin LGTM! Thank you for sharing conversation to some of these refinements. The scripts keep improving! 👾 ✨
| # all: ./scripts/run_validation.sh | ||
| # single: ./scripts/run_validation.sh tests/slack_sdk_async/web/test_web_client_coverage.py | ||
|
|
||
| set -e |
There was a problem hiding this comment.
@WilliamBergamin Thanks for these changes! I like the preference to latest version for formatting so much!
2 participants
Summary
This PR aims to fix warnings in our unit tests and improve the maintainers scripts
Testing
CI should be sufficient
Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.